Conversation
There was a problem hiding this comment.
Thanks @fismif for the detailed description and for calling out the remaining limitations. The root-only scope for this PR makes sense, and overall it looks pretty good!
Turns out, Enclave main doesn't build images as uid 0 currently, which I didn't know because I was never tried it until now 😀
At this point, I'd even split out the fix for the currently broken "build as uid 0" behavior into a separate PR.
Having the guard itself is a major UX improvement over the current build errors that a user would receive wehen trying to run it as root. Having bypass mechanisms is also good and their code looks fine to me.
While reviewing this, I also stumbled across a separate problem, (#132), that we also should fix in a dedicated PR.
| # The job container runs as root, which enclave refuses without an opt-in. | ||
| if enclave tools >/dev/null 2>&1; then | ||
| echo "enclave ran as root without ENCLAVE_ALLOW_ROOT" >&2 | ||
| exit 1 | ||
| fi | ||
| ENCLAVE_ALLOW_ROOT=1 enclave tools >/dev/null |
There was a problem hiding this comment.
This updates the later installation smoke test, but the earlier build-rpm-fedora job also runs as root. Its 'Verify packaged runtime assets' step calls scripts/verify-package-assets.sh, which invokes Enclave twice without an override. CI currently fails at that verification step, so test-fedora is skipped. Please pass --allow-root explicitly to both Enclave invocations in the verifier and use the flag for the allowed invocation here too. The verifier's negative case must get past the guard and still fail because embedded assets are unavailable; a root-refusal error would test the wrong behavior. Please retain the separate assertion here that running without the override is refused.
An out-of scope follow-up issue would be to fix the ci job for fedora. Currently, the fedora ci job runs in a fedora:44 job container and apparently the default user is root there. The follow-up should try setting a non-privileged user in the container.
| For UID 0 (a root host with `--allow-root`, or `--build-uid 0`), the Dockerfile | ||
| adds `agent` as a second name for UID 0 instead of renaming `root`, which | ||
| `usermod` refuses while the build runs as root; the QEMU bundle build does the | ||
| same. Lookups by UID return root's passwd entry, which comes first, so the | ||
| Dockerfile also replaces `/root` with a link to `/home/agent` for `RUN` steps, | ||
| and the runtime sets `HOME` and `USER` for the agent in sessions. |
There was a problem hiding this comment.
I would defer this to a follow-up PR and only do the guard and the --allow-root bypass in this PR without fixing the UID 0 build issue.
Building with uid 0 is broken on main anyway and triggering the guard and an understandable error message is a much nicer UX than the current behavior. If someone then uses --allow-root, they will still get the current broken state, but we can introduce that fix in a separate PR.
| enclave refuses to run as root, including through `sudo`. The agent's container user takes the host UID, so as root the agent runs as UID 0 (named `agent`, a second name for root), which is host root on bind-mounted directories under rootful Docker. Files enclave and the agent write, both in the project and in the stores under the config, state, and cache roots, become root-owned, and later runs as the regular user fail on them (with `sudo -E`, those roots are in the regular user's home). Run enclave as a regular user with access to the Docker socket (see the [requirements](../README.md#requirements)), or use rootless podman with `--backend podman`. | ||
|
|
||
| To run as root anyway, for example in a CI job container, pass `--allow-root` or set `ENCLAVE_ALLOW_ROOT=1`; each such run prints a warning; sessions then run with the agent as UID 0, or as the UID given with `--build-uid`. The opt-in has no config key, so neither global nor project config can grant it. Help and version output work without it. |
There was a problem hiding this comment.
See my comment in ARCHITECTURE.md
| `runningAsRoot` seam. A root host builds the image for UID 0: the Dockerfile | ||
| and the QEMU bundle build add `agent` as a second name for UID 0, and | ||
| `applyUIDZeroAgentEnv` in `internal/runtime` sets `HOME` and `USER` for the | ||
| agent, because lookups by UID return root's passwd entry. To test that path as | ||
| a regular user, pass `--build-uid 0 --build-gid 0` with `XDG_CONFIG_HOME`, | ||
| `XDG_STATE_HOME`, and `XDG_CACHE_HOME` pointing at a scratch directory: the UID 0 | ||
| agent leaves root-owned files behind. |
There was a problem hiding this comment.
See my comment in ARCHITECTURE.md
| // The guard runs before anything writes state (the tool question, asset | ||
| // extraction, stores), so a refused root run leaves no root-owned files. | ||
| // Folding the env opt-in into the options lets validation and per-tool | ||
| // re-resolution see one value. | ||
| parsed.Options.AllowRoot = rootAllowed(parsed.Options.AllowRoot) | ||
| if err := checkRootGuard(parsed.Options.AllowRoot); err != nil { |
There was a problem hiding this comment.
The guard is still reached after writing paths. Run calls discoverUserCommands before parsing, and its ResolveHostHome call creates and deletes a temporary file. More importantly, dynamic completion runs inside cli.Parse: __complete run --tool "" can extract embedded assets, then return before this guard. That leaves a path to root-owned cache files without an opt-in. Please make discovery and the exempt help/version/completion paths read-only, with writing initialization behind the guard. The existing test only checks whether HOME is empty afterward, so it misses temporary writes; please cover completion asset extraction as well. Both behaviors were reproduced with the existing root-check test seam, without running an actual root container.
| if [ "$uid" -eq 0 ]; then | ||
| # busybox adduser refuses a UID in use: add the agent as a second name for root. | ||
| echo "agent:x:0:$gid::/home/agent:/bin/bash" >> /etc/passwd | ||
| echo "agent:!:::::::" >> /etc/shadow | ||
| mkdir -p /home/agent | ||
| chown "0:$gid" /home/agent |
There was a problem hiding this comment.
see my comment in ARCHITECTURE.md
| RUN if [ "${USER_ID}" -eq 0 ]; then \ | ||
| # UID 0 (root host with --allow-root, or --build-uid 0): usermod cannot | ||
| # rename root while the build runs as root, so add the agent as a | ||
| # second name for UID 0 instead. |
There was a problem hiding this comment.
I'm happy for this PR to address only the root-guard part of #106. This alone is a great UX improvement! I would suggest to move the new UID-0 image and runtime support into a separate PR: the Dockerfile and QEMU account changes, the UID-0 HOME/USER handling, and the build-identity extraction needed for that handling.
Those changes introduce separate compatibility questions, including deleting existing /root content and remapping a UID-0 agent. We don't need to solve those here, as the behavior on main is broken anyway in these cases. For this PR, --allow-root or the respective env variable can bypass the new guard without promising to repair previously unsupported root-image workflows; please make that scope clear in the docs. Keep the guard, focused tests/docs, generated flag support, and necessary CI adjustments.
c52fe27 to
f05e4d0
Compare
When run as root, directly or through sudo, enclave and its agent leave root-owned files in the project, and under sudo -E also in the regular user's config, state, and cache roots, so later runs as the regular user fail on them. Under rootful Docker the agent is also host root on every bind-mounted directory. Enclave now refuses to run as root before it writes any state. --allow-root or ENCLAVE_ALLOW_ROOT=1 opts in; there is deliberately no config key, so neither global nor project config can grant it. Help and version output work without the opt-in, the refusal stays on one line so it fits the --json result envelope, and user host commands pass the opt-in on to scripts that re-invoke $ENCLAVE_BIN. The RPM smoke test runs as root in its job container, so it now checks the refusal and then runs with ENCLAVE_ALLOW_ROOT=1. The opt-in has to lead to a working session, but an image for UID 0 could not be built: the Dockerfile renamed the user that already had the build UID to agent, and usermod cannot rename root while the build runs as root. The QEMU bundle build failed too, because busybox adduser refuses a UID in use. Both now add agent as a second passwd name for UID 0 and leave the root entry alone. Lookups by UID still return root's entry, which comes first. RUN steps therefore got HOME=/root, which put the build helpers in /root/.local/bin, off the agent's PATH, so the UID 0 branch replaces /root with a link to /home/agent. Sessions got USER=root and HOME=/root the same way, which made the default git identity root@enclave, so the runtime now exports USER=agent and HOME=/home/agent for UID 0 images, as the QEMU backend already did. Admin sessions keep root's values. The build UID rule moves to model.EffectiveBuildIdentity so the image build and the runtime share it. Non-root builds take the same path as before, but the Dockerfile change alters the image hash, so every image rebuilds once. Tested as root with Docker and QEMU sessions and the refusal, and as a regular user with --build-uid 0. Podman as root and devcontainer remoteUser: root are not covered. Part of eclipse-enclave#106.
Address the review of the root guard. Help, version, and shell completion skip the guard, so nothing before it may write: user command discovery no longer probes HOME for writability, and the completers never extract the embedded assets. A self-contained binary therefore completes tool and feature names only once another command has extracted them. Tests pin directory mtimes under a temporary HOME to catch writes that are undone again, including completion with the real embedded assets. UID 0 image and runtime support moves to a separate change, so the Dockerfile, the QEMU bundle build, and the runtime are back to main; --allow-root now only skips the check. The Fedora RPM jobs run as root, so the package asset check passes --allow-root, and the install smoke test asserts the refusal before running with the flag.
Read-only completion found no app root until a regular command had extracted the embedded assets, so --tool, --features, and the extension remove/update completers offered nothing on a fresh install or after an upgrade. Built-in names now come from the binary in that case, and the installed-extension completer reads only the user root.
What it does
Part of #106 (1 of 2); the sensitive-mount guard follows in a separate PR.
Root guard. Running enclave as root, directly or through
sudo, leaves root-owned files in the project and, undersudo -E, in the regular user's config, state, and cache roots, and later runs as the regular user fail on them. Under rootful Docker the agent is also host root on every bind mount. Enclave now refuses to run as root:internal/app/root_guard.go) runs early inapp.Run, before anything writes state. Help, version, and shell completion still work.--allow-rootorENCLAVE_ALLOW_ROOT=1opts in, and each allowed run prints a warning. There is deliberately no config key, so neither global nor project config can grant it.tools|features add|update|remove --jsonit lands in the result envelope's error field. It names the sudo user when there is one and points to the docker group and rootless podman.$ENCLAVE_BIN.ENCLAVE_ALLOW_ROOT=1.Root sessions work. Getting past the guard was not enough: no image could be built for UID 0 (a root host, or
--build-uid 0).agent, andusermodcannot renamerootwhile the build runs as root. A new UID 0 branch addsagentas a second passwd and group name for UID 0 and leavesrootalone. The existing branches, which non-root builds take, are unchanged.build-bundle.sh): the same approach, because busyboxadduserrefuses a UID in use.RUNsteps therefore gotHOME=/root, and the build helpers landed off the agent'sPATH(enclave-install-tool: not found). The UID 0 branch replaces/rootwith a link to/home/agent.USER=rootandHOME=/rootthe same way, so the default git identity wasroot@enclave. The runtime now exportsUSER=agentandHOME=/home/agentfor UID 0 images, as the QEMU backend already did. Admin sessions keep root's values, andHOME/USERfrom the project.envstill win.effectiveBuildIdentitymoves tomodel.EffectiveBuildIdentity, so the image build and the runtime share the "--build-uid, else the host UID" rule.Worth a close look: in UID 0 images
/rootbecomes a symlink to/home/agent. Root's passwd entry is untouched. On the Debian and Ubuntu bases/rootonly holds.bashrcand.profile, which the agent's home gets from skel; a custom base image with more in/rootwould lose it. Non-root images keep a real/root.Docs: a new "Running as root" section in
docs/cli-reference.md, plusconfiguration.md,security/README.md,windows.md,README.md,ARCHITECTURE.md, andDEV.md.How to test
make testcovers the guard (internal/app/root_guard_test.go), flag parsing (internal/cli/parse_test.go), and the UID 0 session environment (internal/runtime/runtime_devcontainer_test.go).As root, from a throwaway directory (the UID 0 agent can write root-owned files into the project):
Without sudo,
--build-uid 0 --build-gid 0takes the same build path. Point the XDG roots at a scratch directory, because the UID 0 agent leaves root-owned files behind:Leave out
--slimfor now (see Follow-ups).What I verified:
make build,make lint, andmake test. The only failure is ininternal/wslshim, which fails on any host with/usr/bin/enclaveinstalled (see Follow-ups).--build-uid 0:USER=agent,HOME=/home/agent, default git identityagent@enclave, a writable home, and working default features. The admin shell keepsUSER=root.agent, with a real/root.0/0,0/1000,1000/1000, and00/00, the bundle user setup, and a full UID 0 QEMU bundle build.Follow-ups
Related to this change:
--allow-root=falsestill opts in, because bool flags ignore their value (as--verbosedoes). That needs a general parser fix, left out of this PR.sudo enclave --allow-rootin a regular user's checkout. That is git's own protection, and enclave sets nosafe.directory. Should we document it or handle it?remoteUser: root(left out on purpose). QEMU UID 0 bundles don't get the/rootlink; their build doesn't need it, but tools that ignore$HOME, such as ssh, use/rootin the VM.Pre-existing, found while testing:
--slimbuilds fail for every UID with"/extensions/features": not found.prepareBuildContextstages only the selected features, but the Dockerfile copiesextensions/featuresunconditionally.internal/runtime/ide_bridge.go) is nested in the Claude config store, and the container runtime creates itsidemount point there as root on the host. fix: pre-create the tool skills directory in the config store #93 pre-createdskillsandmemory, but notide.TestProbeScriptExitsNotFoundWhenNothingIsInstalledininternal/wslshimfails on hosts with/usr/bin/enclaveinstalled, which is one of the probe's fallback paths.Breaking changes
Anyone who runs enclave as root today is now refused until they pass
--allow-rootor setENCLAVE_ALLOW_ROOT=1. That includes CI job containers,sudo, and WSL distributions created withwsl --import, which log in as root. The policy (refuse by default, a real opt-in, no config key) was agreed with the project lead. Separately, the Dockerfile change alters the image hash, so every image rebuilds once.Review checklist